Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Should fix more but now all "Error in sys.excepthook:" #1069

Merged
merged 5 commits into from
May 21, 2024
Merged

Should fix more but now all "Error in sys.excepthook:" #1069

merged 5 commits into from
May 21, 2024

Conversation

ocefpaf
Copy link
Member

@ocefpaf ocefpaf commented May 13, 2024

Solves some of the Error in sys.excepthook: reported in #1067

In this PR:

  • Remove lru cache: Most of this are cache on top of cache and the slowness were in the get_variables_by_attributes method. That was upstream, I don't see any places that justify keeping caches here. @benjwadams do we have some speed stress test to verify that?
  • Rename ds to nc for consistency.
  • No LRU, no need to clear cache in the base.py, we were playing whack-a-mole there.
  • Removed the unnecessary __dealloc__ method and import do not import Dataset from the private module.

@benjwadams if you agree with these changes please merge and I'll chase the other Error in sys.excepthook in another PR, as far as I can see they are not related to caching but I may be wrong.

@ocefpaf
Copy link
Member Author

ocefpaf commented May 13, 2024

pre-commit.ci autofix

@codecov Codecov
Copy link

codecov bot commented May 13, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 86.04651% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 81.91%. Comparing base (064aac9) to head (6b98a9d).
Report is 20 commits behind head on develop.

❗ Current head 6b98a9d differs from pull request most recent head b137db3. Consider uploading reports for the commit b137db3 to get more accurate results

Files Patch % Lines
compliance_checker/cf/cf_base.py 76.47% 8 Missing ⚠️
compliance_checker/cfutil.py 93.25% 0 Missing and 6 partials ⚠️
compliance_checker/cf/util.py 25.00% 3 Missing ⚠️
compliance_checker/runner.py 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #1069      +/-   ##
===========================================
- Coverage    82.18%   81.91%   -0.28%     
===========================================
  Files           24       24              
  Lines         5171     5171              
  Branches      1242     1237       -5     
===========================================
- Hits          4250     4236      -14     
- Misses         622      634      +12     
- Partials       299      301       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ocefpaf
Copy link
Member Author

ocefpaf commented May 13, 2024

I would ignore that pre-commit failure for now. I cannot reproduce that one locally. Seems to be a bug.

@ocefpaf ocefpaf requested a review from benjwadams May 13, 2024 17:12
@ocefpaf ocefpaf mentioned this pull request May 14, 2024
Closed
@jcermauwedu
Copy link

I still see sys.excepthook errors with this PR.

pytest --cache-clear
================================================================================= test session starts ==================================================================================
platform linux -- Python 3.11.9, pytest-8.2.0, pluggy-1.5.0
rootdir: /home/portal/src/test/compliance-checker
configfile: pyproject.toml
plugins: flake8-1.1.1, requests-mock-1.12.1, time-machine-2.14.1
collected 235 items

compliance_checker/tests/test_acdd.py ................                                                                                                                           [  6%]
compliance_checker/tests/test_base.py ........                                                                                                                                   [ 10%]
compliance_checker/tests/test_cf.py ...........................................................................................                                                  [ 48%]
compliance_checker/tests/test_cf_integration.py .................                                                                                                                [ 56%]
compliance_checker/tests/test_cli.py .........                                                                                                                                   [ 60%]
compliance_checker/tests/test_feature_detection.py ...............................                                                                                               [ 73%]
compliance_checker/tests/test_ioos_profile.py ..........................................                                                                                         [ 91%]
compliance_checker/tests/test_ioos_sos.py ..                                                                                                                                     [ 91%]
compliance_checker/tests/test_protocols.py ....s                                                                                                                                 [ 94%]
compliance_checker/tests/test_suite.py .............                                                                                                                             [ 99%]
compliance_checker/tests/test_util.py .                                                                                                                                          [100%]

====================================================================== 234 passed, 1 skipped in 63.22s (0:01:03) =======================================================================
Error in sys.excepthook:

Original exception was:
Error in sys.excepthook:

Original exception was:
Error in sys.excepthook:

Original exception was:
Error in sys.excepthook:

Original exception was:
Error in sys.excepthook:

Original exception was:
Error in sys.excepthook:

Original exception was:
Error in sys.excepthook:

Original exception was:
Error in sys.excepthook:

Original exception was:

@ocefpaf
Copy link
Member Author

ocefpaf commented May 14, 2024

I still see sys.excepthook errors with this PR.

Yep. Those are the ones related to the mocked netCDF. I still could not get rid of them... At least we reduced them a bit 😬

@ocefpaf
Copy link
Member Author

ocefpaf commented May 14, 2024

@jcermauwedu latest commit squashed the remaining ones. Do you mind taking another look?

BTW, is the segfault gone? I'm concerned about that b/c it can lead to corrupted files!

ocefpaf
ocefpaf commented May 14, 2024
View reviewed changes
compliance_checker/cf/cf_base.py
@@ -495,11 +495,12 @@ def _find_aux_coord_vars(self, ds, refresh=False):
:return: List of variable names (str) that are defined to be auxiliary
coordinate variables.
"""
ds_str = ds.__str__()
Copy link
Member Author

@ocefpaf ocefpaf May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benjwadams this is not elegant but using the actual netCDF4.Dataset object here was leading to odd behaviors. I guess that the str representation of the dataset is unique and "safe" enough for this use. Maybe a hash would be more appropriated, but it tuns hashing netCDF objects is not easy!

@jcermauwedu
Copy link

I will exercise it a bit to see if the segfault appears.

@jcermauwedu
Copy link

After 100 straight iterations, no segfault.

@benjwadams
Copy link
Contributor

@ocefpaf, LRU caching was used because some functions repeatedly used get_variables_by_attributes, considerably slowing down certain datasets, see: #70

In theory, since the datasets are read-only, only one set of attribute fetches should be needed.

I'm OK with regressing performance since the cache implementation appears to be causing issues.

@ocefpaf
Copy link
Member Author

ocefpaf commented May 21, 2024

@ocefpaf, LRU caching was used because some functions repeatedly used get_variables_by_attributes, considerably slowing down certain datasets, see: #70

@benjwadams, like I mentioned above, get_variables_by_attributes caching was upstreamed. If that is the only source of slow downs, then there will be no performance regressions. See Unidata/netcdf4-python#1253

@ocefpaf
Copy link
Member Author

ocefpaf commented May 21, 2024

@benjwadams I'd love to get this one in before the code sprint to avoid code conflicts and to help folks get a dev environment that doesn't segfault.

@benjwadams benjwadams merged commit 1200543 into ioos:develop May 21, 2024
22 of 23 checks passed
@ocefpaf ocefpaf deleted the sys_excepthook branch May 21, 2024 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjwadams benjwadams Awaiting requested review from benjwadams

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ocefpaf @jcermauwedu @benjwadams